Perf: Better response times from api/public/v1/courses #4616

inulty-dfe · 2024-10-24T15:59:35Z

Context

Counting the number of courses in a recruitment cycle is very expensive. We do this on every request to the courses endpoint.

This is an "agonising" endpoint in Skylight

Loadtesting the change

The tests were run with Siege.

Siege documentation

All tests were run against review apps. The review apps all had 1 replica with 512MiB memory.

Before changes

inulty-dfe ~/Code/publish-teacher-training ( perf-api-providers) $ siege -c 10 -t 1m -if app/controllers/api/loadtest-urls-other-branch.txt

{       "transactions":                           91,
        "availability":                       100.00,
        "elapsed_time":                        59.50,
        "data_transferred":                    19.95,
        "response_time":                        5.97,
        "transaction_rate":                     1.53,
        "throughput":                           0.34,
        "concurrency":                          9.13,
        "successful_transactions":                31,
        "failed_transactions":                     0,
        "longest_transaction":                 19.52,
        "shortest_transaction":                 0.97
}71
}

With just `loaded?` changes

inulty-dfe ~/Code/publish-teacher-training ( perf-api-providers) $ siege -c 10 -t 1m -if app/controllers/api/loadtest-urls-no-cache.txt

{       "transactions":                           75,
        "availability":                       100.00,
        "elapsed_time":                        59.49,
        "data_transferred":                    26.39,
        "response_time":                        7.16,
        "transaction_rate":                     1.26,
        "throughput":                           0.44,
        "concurrency":                          9.03,
        "successful_transactions":                45,
        "failed_transactions":                     0,
        "longest_transaction":                 19.76,
        "shortest_transaction":                 1.11
}

With count query cached and `loaded?` changes

inulty-dfe ~/Code/publish-teacher-training ( perf-api-providers) $ siege -c 10 -t 1m -if app/controllers/api/loadtest-urls.txt

{       "transactions":                           94,
        "availability":                       100.00,
        "elapsed_time":                        59.86,
        "data_transferred":                    51.11,
        "response_time":                        6.07,
        "transaction_rate":                     1.57,
        "throughput":                           0.85,
        "concurrency":                          9.53,
        "successful_transactions":                84,
        "failed_transactions":                     0,
        "longest_transaction":                 13.18,
        "shortest_transaction":                 1.90
}

Result

Operation	Original	No cache changes	Cache changes
throughput	0.34	0.44	0.85
successful_transactions	31	45	84
data_transferred	19.95	26.39	51.11

The URLs

The URLs used were variations on this list.
I added no_cache for the "no cache" test run.
I changed the PR number for the "original" test run.

Changes proposed in this pull request

Use loaded? to avoid making requests to the database when we already have records in memory.
Cache the course record count per recruitment cycle for a period of time.
The filter field is not used in the cache key because it is ignored in the query. The API does not filter courses.

Guidance to review

How long should the course count be cached? I would suggest 1 day.

Things to check

gms-gs

Niceee!! <3

app/controllers/api/public/v1/courses_controller.rb

Nitemaeric · 2024-10-31T13:09:07Z

app/models/course.rb

+    return enrichments.select(&:last_published_timestamp_utc).max_by(&:last_published_timestamp_utc)&.last_published_timestamp_utc if enrichments.loaded?
+
    enrichments.maximum(:last_published_timestamp_utc)
  end


This looks like it can be generalised for all associations.

Have you considered looking into ActiveRecord/Associations/CollectionProxy? A #maximum method there might be able to address this issue across the entire app with a single change.

Yeah that looks tempting. but I would be careful about doing that. It's something we should discuss though, it seems like a useful thing to do.

Nitemaeric · 2024-10-31T13:11:25Z

app/controllers/api/public/v1/courses_controller.rb

+        def cached_course_count
+          year = permitted_params[:recruitment_cycle_year] || RecruitmentCycle.current.year
+
+          if permitted_params[:no_cache]


Below might be something for another PR.

It might be more conventional to read the Cache-Control request header and return cached vs non-cached results.

I haven't quite dug into if that's indeed the right header, or what the header value should be (I think no-cache?), and also if my assumption is considered standard practice, but it might be worth looking into.

I'm happy to remove this, I don't think it should be merged. Undocumented api params probably aren't a good idea.

As for the cache headers, I believe those (response headers) are for instructing how the client should cache, not how the server should respond.

I think that this section is indicating that there are request header standards too, but yeah, I haven't seen it in practice yet. I wonder how the browser implements the "Disable cache" checkbox.

But yeah, I agree. Let's remove the no_cache for now.

tomas-stefano · 2024-11-01T15:31:06Z

app/controllers/api/public/v1/courses_controller.rb

+        def cached_course_count
+          year = permitted_params[:recruitment_cycle_year] || RecruitmentCycle.current.year
+
+          Rails.cache.fetch("api_course_count_#{year}", expires_in: 5.minutes) do


Let's have a discussion about this expiration time?

tomas-stefano

Everything looks good. Great initiative. ✌️

Let's have a separate discussion about the cache & expiration.

Need to cache against all params

inulty-dfe added the deploy_v2 label Oct 24, 2024

github-actions bot temporarily deployed to review_aks-4616 October 24, 2024 16:09 Destroyed

github-actions bot temporarily deployed to review_aks-4616 October 24, 2024 16:19 Destroyed

inulty-dfe force-pushed the perf-api-providers branch from 8abbd54 to 292ac6f Compare October 25, 2024 13:21

github-actions bot temporarily deployed to review_aks-4616 October 25, 2024 13:24 Destroyed

github-actions bot temporarily deployed to review_aks-4616 October 25, 2024 15:57 Destroyed

inulty-dfe force-pushed the perf-api-providers branch from 0d446ef to ef56808 Compare October 25, 2024 16:06

github-actions bot temporarily deployed to review_aks-4616 October 25, 2024 16:10 Destroyed

github-actions bot temporarily deployed to review_aks-4616 October 25, 2024 16:32 Destroyed

github-actions bot temporarily deployed to review_aks-4616 October 25, 2024 17:05 Destroyed

inulty-dfe force-pushed the perf-api-providers branch from 9704bcc to fc4cec0 Compare October 28, 2024 09:11

github-actions bot temporarily deployed to review_aks-4616 October 28, 2024 09:15 Destroyed

inulty-dfe force-pushed the perf-api-providers branch from d20fbfb to 7fa9ffd Compare October 28, 2024 11:53

github-actions bot temporarily deployed to review_aks-4616 October 28, 2024 11:58 Destroyed

inulty-dfe force-pushed the perf-api-providers branch from 7fa9ffd to ae1835b Compare October 29, 2024 09:46

github-actions bot temporarily deployed to review_aks-4616 October 29, 2024 09:51 Destroyed

inulty-dfe changed the title ~~Add custom instrumentation to api pagination~~ Perf: Better response times from api/public/v1/courses Oct 30, 2024

github-actions bot temporarily deployed to review_aks-4616 October 30, 2024 12:34 Destroyed

github-actions bot temporarily deployed to review_aks-4616 October 30, 2024 13:21 Destroyed

github-actions bot temporarily deployed to review_aks-4616 October 30, 2024 14:02 Destroyed

github-actions bot temporarily deployed to review_aks-4616 October 30, 2024 16:45 Destroyed

inulty-dfe force-pushed the perf-api-providers branch from fe1da53 to 90c5218 Compare October 30, 2024 17:08

github-actions bot temporarily deployed to review_aks-4616 October 30, 2024 17:11 Destroyed

github-actions bot temporarily deployed to review_aks-4616 October 30, 2024 17:23 Destroyed

github-actions bot temporarily deployed to review_aks-4616 October 31, 2024 09:01 Destroyed

inulty-dfe marked this pull request as ready for review October 31, 2024 09:13

inulty-dfe requested a review from a team as a code owner October 31, 2024 09:13

inulty-dfe force-pushed the perf-api-providers branch from 7e7bb71 to 2c61501 Compare October 31, 2024 09:16

github-actions bot temporarily deployed to review_aks-4616 October 31, 2024 09:21 Destroyed

inulty-dfe force-pushed the perf-api-providers branch from 2c61501 to 3c2cf1f Compare October 31, 2024 09:21

github-actions bot temporarily deployed to review_aks-4616 October 31, 2024 09:27 Destroyed

gms-gs approved these changes Oct 31, 2024

View reviewed changes

Nitemaeric reviewed Oct 31, 2024

View reviewed changes

app/controllers/api/public/v1/courses_controller.rb Outdated Show resolved Hide resolved

app/controllers/api/public/v1/courses_controller.rb Show resolved Hide resolved

app/controllers/api/public/v1/courses_controller.rb Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to review_aks-4616 October 31, 2024 10:08 Destroyed

inulty-dfe force-pushed the perf-api-providers branch from ef44f71 to ec08b4d Compare October 31, 2024 10:30

github-actions bot deployed to review_aks-4616 October 31, 2024 10:33 View deployment

inulty-dfe requested review from Nitemaeric and a team October 31, 2024 10:52

Nitemaeric reviewed Oct 31, 2024

View reviewed changes

inulty-dfe force-pushed the perf-api-providers branch from ec08b4d to 57a9250 Compare November 1, 2024 09:35

tomas-stefano reviewed Nov 1, 2024

View reviewed changes

tomas-stefano approved these changes Nov 1, 2024

View reviewed changes

Nitemaeric assigned inulty-dfe Nov 5, 2024

inulty-dfe added 8 commits December 2, 2024 15:28

Reduce number of queries in the courses api

f29df41

Cache the course count in the api

a3b1200

Cache year

0907c46

Need to cache against all params

Filter out enrichments where the last_published_timestamp_utc is nil

02f294e

Add test for when course.enrichments are not loaded

673655c

Add spec for Course#last_published_at

fe478be

Specify that api/courses params are permitted now

56988d8

Remove no_cache api param

6a711b8

inulty-dfe force-pushed the perf-api-providers branch from 57a9250 to 6a711b8 Compare December 2, 2024 15:30

inulty-dfe merged commit d85cc05 into main Dec 2, 2024
19 checks passed

inulty-dfe deleted the perf-api-providers branch December 2, 2024 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perf: Better response times from api/public/v1/courses #4616

Perf: Better response times from api/public/v1/courses #4616

inulty-dfe commented Oct 24, 2024 •

edited

Loading

gms-gs left a comment

Nitemaeric Oct 31, 2024

inulty-dfe Oct 31, 2024

Nitemaeric Oct 31, 2024

inulty-dfe Oct 31, 2024

Nitemaeric Oct 31, 2024

tomas-stefano Nov 1, 2024

tomas-stefano left a comment

Perf: Better response times from api/public/v1/courses #4616

Perf: Better response times from api/public/v1/courses #4616

Conversation

inulty-dfe commented Oct 24, 2024 • edited Loading

Context

This is an "agonising" endpoint in Skylight

Loadtesting the change

Before changes

With just loaded? changes

With count query cached and loaded? changes

Result

The URLs

Changes proposed in this pull request

Guidance to review

Things to check

gms-gs left a comment

Choose a reason for hiding this comment

Nitemaeric Oct 31, 2024

Choose a reason for hiding this comment

inulty-dfe Oct 31, 2024

Choose a reason for hiding this comment

Nitemaeric Oct 31, 2024

Choose a reason for hiding this comment

inulty-dfe Oct 31, 2024

Choose a reason for hiding this comment

Nitemaeric Oct 31, 2024

Choose a reason for hiding this comment

tomas-stefano Nov 1, 2024

Choose a reason for hiding this comment

tomas-stefano left a comment

Choose a reason for hiding this comment

inulty-dfe commented Oct 24, 2024 •

edited

Loading

With just `loaded?` changes

With count query cached and `loaded?` changes